-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http2: fix issues with aborted respondWithFile()
s
#21561
Conversation
New CI: https://ci.nodejs.org/job/node-test-pull-request/15651/ @ariran5 Can you download the source for v10.5.0, apply https://patch-diff.githubusercontent.com/raw/nodejs/node/pull/21561.diff and build Node.js yourself? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code SGTM but I couldn't reproduce on my Mac so I can't confirm the fix.
b41d6f6
to
9d7dbb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax cool, could not reproduce crashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax I'll check if the test I wrote still crashes (highly unlikely), but code-wise LGTM.
Thanks for the reviews – I’m labelling this as |
@addaleax I think you're right that a no-op error handler on the client side should help with the flakiness. |
I can reproduce the failure reliably but don't know how to fix. The test as is doesn't seem to work on MacOS since the destroy causes the error. |
Yes! Its work for me, fast reload not crush node |
Could somebody from e.g. @nodejs/platform-macos help me figure out the test here? I don’t have easy access to any of the systems on which is is failing… |
Maybe ping @nodejs/http2 ? I guess alternatively I’ll ask for access to a build machine … |
FWIW I tried adding the following handler to the server in the test case. On macOS High Sierra (10.13.6) it fails to emit the stream.on('error', common.mustCall((error) => {
const expected = 'ECONNRESET'
assert.strictEqual(error.errno, expected);
assert.strictEqual(error.code, expected);
})); |
New CI to see on which hosts exactly it’s failing (?): https://ci.nodejs.org/job/node-test-pull-request/15809/ |
The socket destroy is what causes the (The test might need to be rewritten to work on macOS from what I can tell.) |
9d7dbb9
to
bb4e94a
Compare
I’ve pushed a change that might help get this back on track, even if it’s just a workaround. If this is approved & lands with the hack in the test, I’ll open a new issue, since I think this is an API defect in the HTTP/2 API. (@apapirovski I still really appreciate you taking the time to do this – I’d assign that issue to you, if that’s okay, at least as long as I can’t really debug it myself. :/) |
CI is green. Can somebody take a look at the added changes (only 6a04dbe)? I think it’s okay to do this because it addresses the most important issue, which is that this should not hard-crash the process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed in d94950e |
Fixes: #20824
Fixes: #21560
@budarin @DaAitch @ariran5 Are you in a position to try this patch and see whether it fully resolves the issues you were experiencing?
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes